Paper 26: async spawn rescue, Paper API build, DummyChunk, and WorldEdit-safe command init#373
Paper 26: async spawn rescue, Paper API build, DummyChunk, and WorldEdit-safe command init#373rkfsociety wants to merge 3 commits into
Conversation
Replace deprecated PlayerSpawnLocationEvent; run world access on the main thread via callSyncMethod. Compile against paper-api 26.1.2.build.60-stable. Update DummyChunk for current Chunk API (load level, structures, tile entities, four-arg chunk snapshot).
Defer FlagHandler construction until first flag/unflag command so WorldEdit classes (e.g. SessionOwner) are on the classpath. Add WorldEdit to plugin softdepend for ordering when Bukkit resolves dependencies.
Jikoo
left a comment
Jikoo
left a comment
There was a problem hiding this comment.
Re: WorldEdit - WG depends on WE, so this should not be an issue unless you have dependency loops entirely breaking load order. You should probably check your other plugins if you're having problems.
If you are interested in fixing the problems with it, please submit it as a separate PR.
Re: AsyncPlayerSpawnLocationEvent: I'm not interested in dropping Spigot compatibility. I would be fine with a Spigot and Paper module, but I am not interested in an AI PR doing the required modularization because I would probably go insane wasting the time reviewing that much weird code.
In general, please try to actually look at what your AI did.
| FlagHandler local = this.flagHandler; | ||
| if (local == null) { | ||
| synchronized (this) { | ||
| local = this.flagHandler; | ||
| if (local == null) { | ||
| local = new FlagHandler(this.plugin); | ||
| this.flagHandler = local; | ||
| } | ||
| } | ||
| } | ||
| return local; |
There was a problem hiding this comment.
Locking is not really necessary here. Commands are processed on the main thread. Even if they weren't (i.e. maybe Folia, which also isn't supported because we use the regular Bukkit scheduler), the odds of two players just so happening to execute commands both requiring a flag handler at the same exact time are incredibly small.
I'm way more concerned with the fact that AI thinks using a publicly-accessible instance for a lock is a good idea. Good lord.
| /** | ||
| * Lazily created: {@link FlagHandler} touches WorldEdit types. If we construct it from | ||
| * {@link Regionerator#onEnable()} before WorldEdit has enabled, {@code SessionOwner} etc. are not | ||
| * visible yet and the plugin fails with {@link NoClassDefFoundError}. | ||
| */ |
There was a problem hiding this comment.
I'd prefer docs on the method to the field.
| * {@link Regionerator#onEnable()} before WorldEdit has enabled, {@code SessionOwner} etc. are not | ||
| * visible yet and the plugin fails with {@link NoClassDefFoundError}. | ||
| */ | ||
| private volatile @Nullable FlagHandler flagHandler; |
There was a problem hiding this comment.
Volatile and a lock? Unnecessary, I would drop the keyword.
/e: Ah, it is necessary due to how the AI set it up. Eugh.
| Player player = event.getPlayer(); | ||
| // Check if the player has logged in since this feature was added. | ||
| PersistentDataContainer playerPdc = player.getPersistentDataContainer(); | ||
| if (!playerPdc.has(loggedInSinceFeature, PersistentDataType.BYTE)) { | ||
| playerPdc.set(loggedInSinceFeature, PersistentDataType.BYTE, (byte) 1); |
There was a problem hiding this comment.
Quick returns before incurring a scheduler hit should be re-added. PDC logic does not need to wait for main thread.
| Chunk chunk = spawnLoc.getChunk(); | ||
| PersistentDataContainer chunkPdc = chunk.getPersistentDataContainer(); | ||
| NamespacedKey logoutKey = getLogoutKey(player.getUniqueId()); | ||
| // If the key is set, the chunk has not been deleted since the player last logged out. |
…aper-only API Restore spigot-api build, revert Chunk API changes, and remove WorldEdit load-order workaround from this PR. Register Paper AsyncPlayerSpawnLocationEvent handler via reflection when available while keeping PlayerSpawnLocationEvent for Spigot.
|
Look, I'm gonna be honest, there are a lot of things that are slightly wrong and rather weird with these changes, and I don't really feel like wasting my time giving you feedback to feed into an AI when I could just prompt an AI myself and cut out the middleman. If you want to PR lazy access for WE, feel free. Like I said before, the risk of concurrency issues are almost nonexistent even if a platform were to implement async command processing, so it really doesn't need to be complex. |
|
I basically don't understand programming, I just solved this problem with the help of AI, and after a week I had no errors, I decided to share with the developer, if any of this is useful, use it, if not, then so be it. |
|
I appreciate the attempt to contribute a fix, but I hope you can see where I'm coming from. If I have to re-prompt through you, we might be here for weeks with communication delays. If I have to rewrite it myself, why would I not just do that? The biggest problem with both of your iterations is a slightly subtle bug: they effectively undo the async part of the AsyncPlayerSpawnLocationEvent. Add in the extra player data read, and suddenly if anything it's going to have worse performance. Sure, you might not get warnings about the event being listened to, but that's really not super critical unless the event is actually scheduled to be removed, which it isn't. The other problems are more me-problems: The AI does some weird things (like that abuse of an array for the location callback, ew), and it didn't make any effort to minimize the diff. It's hard to read and process. For a human author who would learn and grow as a developer, me taking the time to offer suggestions to fix that is an investment in the quality of the community. For an AI author, well, that's just what that model does, and I'm wasting my breath because the next person will be starting from a blank slate with it. |
2 participants
This branch contains two logical changes (two commits).
1) Paper 26 spawn / rescue + build +
DummyChunkPlayerSpawnLocationEventwithAsyncPlayerSpawnLocationEventinRescueListener.BukkitScheduler#callSyncMethod.runTaskafter thePlayerexists (same intent as before).OfflinePlayer#getRespawnLocation(true)for respawn-based rescue.io.papermc.paper:paper-api26.1.2.build.60-stable(the26.1.2-R0.1-SNAPSHOTcoordinate is not on Paper’s Maven).DummyChunkfor the currentChunkAPI (getLoadLevel,isGenerated,getStructures,getPlayersSeeingChunk, four-arggetChunkSnapshot,getTileEntitiesoverloads) and fixisLoaded()(chunkX/chunkZ).2) WorldEdit classpath / load order
FlagHandlerpulls in WorldEdit types. Creating it fromRegioneratorExecutorduringonEnablecould run before WorldEdit finished enabling and triggeredNoClassDefFoundErrorforcom.sk89q.worldedit.session.SessionOwner.FlagHandlercreation on first/regionerator flagorunflag, and addWorldEdittoplugin.ymlsoftdepend.